Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow passing custom key to BatchLoader#batch #12

Merged
merged 4 commits into from
Nov 16, 2017
Merged

Conversation

exAspArk
Copy link
Owner

No description provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.721% when pulling 6be8061 on dlanileonardo-master into a529396 on master.

@exAspArk exAspArk mentioned this pull request Nov 13, 2017
@default_value = default_value
@block = block
@block_hash_key = block.source_location
@block_hash_key = key || block.source_location
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm overthinking this, but I can see a case for wanting the key to be key.to_s + block.source_location (allows the key to be unique on a per loader basis, rather than codebase wide) and I cannot see a case for not wanting it to be key.to_s + block.source_location

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's a great idea!

Even though with metaprogramming (eval) key should be still unique through the whole codebase, in rest of the cases it is very useful and may help avoid potential collisions.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 99.721% when pulling ba887f3 on dlanileonardo-master into a529396 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 99.72% when pulling 7807dad on dlanileonardo-master into a529396 on master.

@exAspArk exAspArk merged commit 95ff019 into master Nov 16, 2017
@exAspArk exAspArk deleted the dlanileonardo-master branch November 16, 2017 16:21
@exAspArk
Copy link
Owner Author

Released it in the 1.2.0 version.
Thank you guys for your contribution and help! 🎉

@mathieul
Copy link

Thank you for all the work you put into this gem and thank you to all who worked on this PR. It now allows to generate code to avoid boilerplate in common patterns, so I wrote a mixin using batch-loader to generate lazy active record associations.

I've also packaged it as a gem in case it is useful to others: https://github.com/mathieul/batch-loader-active-record.

@exAspArk
Copy link
Owner Author

@mathieul wow, looks awesome! 😍

I remember some people on Reddit were willing to have batch-loader implemented in ActiveRecord. Your gem finally allows to integrate them together!

Thank you for sharing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants